Scan workspaces in background tasks#1245
Conversation
lionel-
force-pushed
the
oak/salsa-12-scan-workspace
branch
from
48eb900 to
a29744e
Compare
lionel-
force-pushed
the
oak/salsa-13-scan-async
branch
from
1abf718 to
6c0c303
Compare
| pub(super) fn dispatch_scan_requests( | ||
| events_tx: &TokioUnboundedSender<Event>, | ||
| requests: Vec<ScanRequest>, | ||
| ) { | ||
| for req in requests { | ||
| let tx = events_tx.clone(); | ||
| spawn_blocking(move || { | ||
| let scan = req.run(); | ||
| tx.send(Event::OakScanCompleted(scan)).log_err(); | ||
| Ok(None) | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Rather than tweaking the value of each LSP Request to possibly return ScanRequests, I think I would much rather allow state_handlers::initialize() and state_handlers::did_change_workspace_folders() to kick off the scan requests themselves.
i.e. if you just pass events_tx to each of those, they should have everything they need to kick off a spawn_blocking() call.
That way you can, for example, keep the dispatch close to the lsp_state.oak_scheduler.set_workspace_paths() call that requires it, which seems nice to me.
I also really like that that would mean that we would not need the respond_with() changes (I'm not super comfortable with how the ::default() behavior there works) and tweaks to the return values just for this one feature.
There was a problem hiding this comment.
(This one feels important enough to me to talk about)
There was a problem hiding this comment.
Seems like this idea has merit, with dispatch_scan_requests() calls moving right after oak_scheduler usage like this
and then this nicely simplifies back to where we started
| self.state.insert(root, ScanState::Scanning); | ||
| let Some(path) = root.path(db).to_file_path().warn_on_err() else { | ||
| return Vec::new(); | ||
| }; | ||
| vec![ScanRequest { root, path }] |
There was a problem hiding this comment.
The early return Vec::new() seems like a problem. You set this root up as Scanning but if you don't return a ScanRequest for it, then it will never get scanned and will be infinitely left in Scanning mode
There was a problem hiding this comment.
do you want to do this there?
self.state.remove(&root);
self.buffered.remove(&root);
a29744e to
eec3212
Compare
lionel-
force-pushed
the
oak/salsa-13-scan-async
branch
2 times, most recently
from
8f13f9c to
5f3ecf2
Compare
lionel-
force-pushed
the
oak/salsa-13-scan-async
branch
from
ec1435d to
a2787c2
Compare
2 participants
Branched from #1244
Progress towards #1212
This PR moves workspace scanning off the LSP main loop, to avoid blocking frontend requests on initialize and on every
didChangeWorkspaceFolders. This PR introduces the same setuprust-analyzerandtyuse: the handler dispatches scanning work to an asynchronous task pool and replies immediately.The new piece is
oak_scan::ScanScheduler, which owns the policy (when to scan, how to handle events arriving mid-scan) but not the runtime. The LSP layer is free to use any appropriate mechanism for running off-thread tasks: The LSP takes aScanRequestfrom theoak_scanscheduler, run it viaScanRequest::run()off-thread task, and hand the resultingScanCompletedresults back tooak_scanviaScanScheduler::apply_scan_completed().In Ark's case we spawn each request on
lsp::spawn_blocking()(which handles things like crashes gracefully with frontend reports) and ships the result back via a newEvent::OakScanCompletedfor the main loop. Scheduler state and Salsa inputs are mutated only on the main loop, which reduces the surface for race handling issues. We just need to ensure proper event ordering inoak_scan, there are no issues of concurrent accesses at all in the LSP layer.See module doc in
scheduler.rsfor how race conditions are resolved.